feat: initial keyboard shortcut support #263

gkatsev · 2022-07-11T21:05:18Z

Kicking off the work for #257

vercel · 2022-07-11T21:17:03Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
media-chrome	✅ Ready (Inspect)	Visit Preview	Jul 22, 2022 at 7:07PM (UTC)

src/js/media-container.js

cjpillsbury

a couple of asks, but this looks close to done for a good mvp spike.

src/js/media-container.js

src/js/media-controller.js

gkatsev · 2022-07-12T22:46:55Z

src/js/media-controller.js

+/**
+ * Media Controller should not mimic the HTMLMediaElement API.
+ * @see https://github.com/muxinc/media-chrome/pull/182#issuecomment-1067370339
+ */


moved from the bottom of the class here, which seems more appropriate.

src/js/media-controller.js

src/js/media-container.js

cjpillsbury

One docs chore and a handful of non-blocking callouts. Otherwise, 👌. Did basic smoke testing across available browsers.

cjpillsbury · 2022-07-21T14:00:14Z

src/js/media-controller.js

-   * @see https://github.com/muxinc/media-chrome/pull/182#issuecomment-1067370339
-   */
+  keyboardShortcutHandler(e) {
+    if (this.contains(e.target) || this.shadowRoot.contains(e.target)) {


question(non-blocking): Do we need these checks?

Maybe not, since I guess the event bubbling should guarantee that it's coming from a child element?

yup that's what I was thinking. We may also get into some weirdness with contains checks for nested web component use cases.

cjpillsbury · 2022-07-21T14:03:54Z

src/js/media-controller.js

+      // keysUsed is either an attribute or a property.
+      // The attribute is a DOM array and the property is a JS array
+      // In the attribute Space represents the space key and gets convered to ' '
+      const keysUsed = (e.target.getAttribute('keysused')?.split(' ') ?? e.target?.keysUsed ?? [])


thought(non-blocking): As a followup, we'll need to figure out how this should work for non-nested components based on the current architecture (See code/architecture for associatedElements and how event (re)propagation works).

yup, this got called out in the discussion #257

cjpillsbury · 2022-07-21T14:08:44Z

src/js/media-controller.js

+        return;
+      }
+
+      if (!this.hasAttribute('nohotkeys')) {


suggestion(non-blocking): Instead of continuing to listen to irrelevant keyboard presses when "nohotkeys" is set, we should probably:

add "nohotkeys" to observedAttributes.

add/remove event handlers for "keyup"/"keydown" based on attributeChangedCallback

This entails moving the event callbacks to methods instead of inline.

cjpillsbury · 2022-07-21T14:13:19Z

docs/media-controller.md

+| ⬅       | Seek back 10s     |
+| ➡       | Seek forward 10s  |
+
+If you are implementing an interactive element that uses any of these keys, you can stopPropagation in your `keyup` handler. Alternatively, you can add a `keysUsed` property on the element or a `keysused` attribute.


chore(blocking):

Discuss and link to https://developer.mozilla.org/en-US/docs/Web/API/UI_Events/Keyboard_event_key_values

Callout the "Space" keyword mapping

cjpillsbury · 2022-07-22T18:48:02Z

docs/media-controller.md

+If you are implementing an interactive element that uses any of these keys, you can stopPropagation in your `keyup` handler. Alternatively, you can add a `keysUsed` property on the element or a `keysused` attribute. The values are those that match the `key` property on the KeyboardEvent. You can find a list of those values [on mdn](https://developer.mozilla.org/en-US/docs/Web/API/UI_Events/Keyboard_event_key_values). Additionally, since the DOM list can't have the Space key represented as `" "`, we will accept `Space` as an alternative name for it.
+Example (`keysused` attribute):
+```html
+   <media-time-range keysused="ArrowLeft ArrowRight Space"></media-time-range>


cjpillsbury

pre-approving with the minor callout and recommend to remove the contains et al. check.

cjpillsbury · 2022-07-22T19:00:14Z

src/js/media-controller.js

+  #keyUpHandler(e) {
+    const { key } = e;
+    if (!ButtonPressedKeys.includes(key)) {
+      this.removeEventListener('keyup', keyUpHandler);


issue(blocker): missed this.#keyUpHandler refactor.

luwes · 2022-07-22T19:24:04Z

docs/media-controller.md

+Example (hotkeys disabled):
+
+```html
+<media-controller nohotkeys>


luwes

LGTM! 🎉

gkatsev added 2 commits July 11, 2022 17:04

feat: initial keyboard shortcut support

b32326a

vercel

653e7d6

vercel bot deployed to Preview July 11, 2022 21:17 View deployment

gkatsev commented Jul 11, 2022

View reviewed changes

src/js/media-container.js Outdated Show resolved Hide resolved

cjpillsbury requested changes Jul 11, 2022

View reviewed changes

src/js/media-container.js Outdated Show resolved Hide resolved

src/js/media-container.js Outdated Show resolved Hide resolved

src/js/media-container.js Outdated Show resolved Hide resolved

heff reviewed Jul 12, 2022

View reviewed changes

src/js/media-container.js Outdated Show resolved Hide resolved

gkatsev added 3 commits July 12, 2022 18:37

move keyboard shortcut handling into media-controller

9ee35c4

add a way to turn off keyboard shortcuts

569eff8

move default seek offset to a file VARIABLE

bd96d27

vercel bot deployed to Preview July 12, 2022 22:44 View deployment

gkatsev commented Jul 12, 2022

View reviewed changes

feat: expose keyUsed from interactive elements

324a465

vercel bot deployed to Preview July 18, 2022 21:46 View deployment

fix: rename noshortcuts to nohotkeys

1d08679

vercel bot deployed to Preview July 18, 2022 21:47 View deployment

feat: allow keysused attribute to be used on interactive elements

56b60a7

vercel bot deployed to Preview July 20, 2022 23:54 View deployment

simplify keysused code

c79aaef

vercel bot deployed to Preview July 21, 2022 00:57 View deployment

add docs for keyboard shortcuts

cc6af1a

vercel bot deployed to Preview July 21, 2022 01:08 View deployment

cjpillsbury requested changes Jul 21, 2022

View reviewed changes

docs: update docs to add keys and mention Space-' ' mapping

8c10b0c

vercel bot deployed to Preview July 22, 2022 18:27 View deployment

fix: toggle key handlers based on nohotkeys attribute changing

b2f212d

vercel bot deployed to Preview July 22, 2022 18:44 View deployment

cjpillsbury reviewed Jul 22, 2022

View reviewed changes

cjpillsbury approved these changes Jul 22, 2022

View reviewed changes

fix: private method

38f65f8

remove check for contains

b9d63e0

vercel bot deployed to Preview July 22, 2022 19:07 View deployment

luwes reviewed Jul 22, 2022

View reviewed changes

docs/media-controller.md

Example (hotkeys disabled):

```html

<media-controller nohotkeys>

Copy link

Contributor

luwes Jul 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

luwes approved these changes Jul 22, 2022

View reviewed changes

gkatsev merged commit 7542ce7 into muxinc:main Jul 22, 2022

gkatsev deleted the keyboard-shortcuts-poc branch July 22, 2022 20:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: initial keyboard shortcut support #263

feat: initial keyboard shortcut support #263

gkatsev commented Jul 11, 2022

vercel bot commented Jul 11, 2022 •

edited

Loading

cjpillsbury left a comment

gkatsev Jul 12, 2022

luwes Jul 22, 2022

cjpillsbury left a comment

cjpillsbury Jul 21, 2022

gkatsev Jul 22, 2022

cjpillsbury Jul 22, 2022

cjpillsbury Jul 21, 2022

gkatsev Jul 22, 2022

cjpillsbury Jul 21, 2022

gkatsev Jul 22, 2022

cjpillsbury Jul 21, 2022

gkatsev Jul 22, 2022

cjpillsbury Jul 22, 2022

cjpillsbury left a comment

cjpillsbury Jul 22, 2022

luwes Jul 22, 2022

luwes left a comment

feat: initial keyboard shortcut support #263

feat: initial keyboard shortcut support #263

Conversation

gkatsev commented Jul 11, 2022

vercel bot commented Jul 11, 2022 • edited Loading

cjpillsbury left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjpillsbury left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjpillsbury left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luwes left a comment

Choose a reason for hiding this comment

vercel bot commented Jul 11, 2022 •

edited

Loading